✨ Feature: restrict API Server LB access via IPs #1247

bavarianbidi · 2022-05-23T11:21:35Z

Access to API-Server can be restricted by setting allowedCidrs on spec.APIServerLoadBalancer.AllowedCIDRs.

Behavior:

outgoing NAT Router-IPs is now part of the status:

cluster-api-provider-openstack/pkg/cloud/services/networking/router.go

Lines 73 to 83 in 9830f57

    
           routerIPs := []string{} 
        
           for _, ip := range router.GatewayInfo.ExternalFixedIPs { 
        
           	routerIPs = append(routerIPs, ip.IPAddress) 
        
           } 
        
           openStackCluster.Status.Network.Router = &infrav1.Router{ 
        
           	Name: router.Name, 
        
           	ID:   router.ID, 
        
           	Tags: router.Tags, 
        
           	IPs:  routerIPs, 
        
           }

once spec.APIServerLoadBalancer.AllowedCIDRs changed (create/update), CAPO will fetch all known required IP addresses from spec and status and will generate a new list of allowed CIDRs

cluster-api-provider-openstack/pkg/cloud/services/loadbalancer/loadbalancer.go

Lines 193 to 207 in 9830f57

    
           if len(openStackCluster.Spec.APIServerLoadBalancer.AllowedCIDRs) > 0 { 
        
           	allowedCIDRs = append(allowedCIDRs, openStackCluster.Spec.APIServerLoadBalancer.AllowedCIDRs...) 
        
           	if openStackCluster.Spec.Bastion.Enabled { 
        
           		allowedCIDRs = append(allowedCIDRs, openStackCluster.Status.Bastion.FloatingIP, openStackCluster.Status.Bastion.IP) 
        
           	} 
        
           	if openStackCluster.Status.Network.Subnet.CIDR != "" { 
        
           		allowedCIDRs = append(allowedCIDRs, openStackCluster.Status.Network.Subnet.CIDR) 
        
           	} 
        
           	if len(openStackCluster.Status.Network.Router.IPs) > 0 { 
        
           		allowedCIDRs = append(allowedCIDRs, openStackCluster.Status.Network.Router.IPs...) 
        
           	} 
        
           }

after allowed_cidrs on OpenStack are updated on the corresponding listener, the list of applied CIDRs will be written back to the status

cluster-api-provider-openstack/pkg/cloud/services/loadbalancer/loadbalancer.go

Lines 113 to 119 in 9830f57

    
           openStackCluster.Status.Network.APIServerLoadBalancer = &infrav1.LoadBalancer{ 
        
           	Name:         lb.Name, 
        
           	ID:           lb.ID, 
        
           	InternalIP:   lb.VipAddress, 
        
           	IP:           lbFloatingIP, 
        
           	AllowedCIDRs: allowedCIDRs, 
        
           }

Conclusion:

only one additional request towards openstack - whenever the on OpenSTack applied CIDRs differes from the spec
spec.APIServerLoadBalancer.AllowedCIDRs is now mutable

Open questions:

IP/CIDR validation during cluster-spec editing ?
current implementation doesn't validate IPs during creation/editing of cluster-spec. IP validation will be done afterwards and non-valid IPs will get removed.
Is this ok or should we already validate the CIDRs in the webhook?

Signed-off-by: Mario Constanti [email protected]

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1045

Special notes for your reviewer:

With

cluster-api-provider-openstack/api/v1alpha4/conversion.go

Lines 36 to 40 in 9830f57

    
           // Manually restore data. 
        
           restored := &infrav1.OpenStackCluster{} 
        
           if ok, err := utilconversion.UnmarshalData(r, restored); err != nil || !ok { 
        
           	return err 
        
           }

and

cluster-api-provider-openstack/api/v1alpha4/conversion.go

Lines 62 to 65 in 9830f57

    
           // Preserve Hub data on down-conversion except for metadata 
        
           if err := utilconversion.MarshalData(src, r); err != nil { 
        
           	return err 
        
           }

we will now get the cluster.x-k8s.io/conversion-data Annotation if we query CAPO-related resources in a previous CAPO-Version.

This is currently missing but needed in general.
E.g. compare the output of k get cluster.v1alpha4.cluster.x-k8s.io and k get cluster.v1beta1.cluster.x-k8s.io where conversion is fine versus CAPO - e.g. k get osc.v1alpha4.infrastructure.cluster.x-k8s.io where we don't have the cluster.x-k8s.io/conversion-data annotation yet.

xref: SIG Cluster Lifecycle - ClusterAPI - API conversion code walkthrough

TODOs:

squashed commits
includes documentation
adds unit tests

/hold

netlify · 2022-05-23T11:21:39Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`e78f0b8`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/62a6ce30ed7f740008cd0542
😎 Deploy Preview	https://deploy-preview-1247--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

k8s-ci-robot · 2022-05-23T11:21:43Z

Hi @bavarianbidi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jichenjc · 2022-05-23T12:16:43Z

/ok-to-test

jichenjc · 2022-05-27T03:26:25Z

/test pull-cluster-api-provider-openstack-test

bavarianbidi · 2022-05-27T18:32:44Z

@bavarianbidi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-openstack-test 12abd16 link true /test pull-cluster-api-provider-openstack-test
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

wondering why this test doesn't fail on my local machine 🤷 will fix it on monday

jichenjc · 2022-05-30T03:52:38Z

looks like this is Fuzzy test failure :) so need check the Convert function failure
so local test might pass because of lucky ?

bavarianbidi · 2022-05-30T04:59:23Z

looks like this is Fuzzy test failure :) so need check the Convert function failure so local test might pass because of lucky ?

had to rebase again, as openstackclustertemplate conversion was added 5 days ago. I guess some caching issues in the test-instance ¯\_(ツ)_/¯

seanschneeweiss

Very nice. Thanks a lot for the documentation too. This is a great feature.

api/v1alpha5/types.go

pkg/cloud/services/loadbalancer/loadbalancer.go

pkg/utils/strings/strings.go

docs/book/src/clusteropenstack/configuration.md

pkg/cloud/services/loadbalancer/loadbalancer.go

docs/book/src/clusteropenstack/configuration.md

pkg/cloud/services/loadbalancer/loadbalancer.go

docs/book/src/clusteropenstack/configuration.md

tobiasgiese

Thanks for this great contribution! 👌🏻

pkg/cloud/services/loadbalancer/loadbalancer.go

pkg/utils/openstack/loadbalancer.go

* Access to API-Server can be restricted by setting allowedCidrs on spec.APIServerLoadBalancer.AllowedCIDRs field * Default LB provider is set to amphora if multiple providers exist. Features like access restriction via IPs is only supported in amphora. * Outgoing NAT Router-IPs are now part of the OpenStackCluster status. * To support the entire feature set of conversion, the cluster.x-k8s.io/conversion-data annotation got introduced if quering objects in a previous api version. * make generate now regenerate the mock loadbalancer_service client if needed Signed-off-by: Mario Constanti <[email protected]>

jichenjc · 2022-06-13T08:52:21Z

/lgtm

tobiasgiese · 2022-06-13T08:54:04Z

/approve

k8s-ci-robot · 2022-06-13T08:54:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bavarianbidi, tobiasgiese

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tobiasgiese]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bavarianbidi · 2022-06-15T13:07:48Z

/unhold

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 23, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 23, 2022

k8s-ci-robot requested review from seanschneeweiss and tobiasgiese May 23, 2022 11:21

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 23, 2022

bavarianbidi mentioned this pull request May 23, 2022

IP Whitelisting for API server in CAPO giantswarm/roadmap#921

Closed

2 tasks

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 23, 2022

apricote mentioned this pull request May 25, 2022

Passing security groups by specifying more options in addition to UUIDs on ports #1246

Merged

bavarianbidi force-pushed the firewall_restriction_on_api_lb branch from 12abd16 to 8922a62 Compare May 30, 2022 04:58

seanschneeweiss reviewed Jun 1, 2022

View reviewed changes

jichenjc reviewed Jun 1, 2022

View reviewed changes

docs/book/src/clusteropenstack/configuration.md Show resolved Hide resolved

docs/book/src/clusteropenstack/configuration.md Outdated Show resolved Hide resolved

pkg/cloud/services/loadbalancer/loadbalancer.go Outdated Show resolved Hide resolved

apricote reviewed Jun 2, 2022

View reviewed changes

docs/book/src/clusteropenstack/configuration.md Outdated Show resolved Hide resolved

pkg/cloud/services/loadbalancer/loadbalancer.go Outdated Show resolved Hide resolved

docs/book/src/clusteropenstack/configuration.md Show resolved Hide resolved

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 8, 2022

bavarianbidi requested review from apricote, jichenjc and seanschneeweiss June 8, 2022 12:07

tobiasgiese reviewed Jun 12, 2022

View reviewed changes

pkg/cloud/services/loadbalancer/loadbalancer.go Outdated Show resolved Hide resolved

pkg/cloud/services/loadbalancer/loadbalancer.go Show resolved Hide resolved

pkg/utils/openstack/loadbalancer.go Show resolved Hide resolved

bavarianbidi force-pushed the firewall_restriction_on_api_lb branch from 463c45f to e78f0b8 Compare June 13, 2022 05:42

bavarianbidi requested a review from tobiasgiese June 13, 2022 05:46

k8s-ci-robot assigned jichenjc Jun 13, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2022

k8s-ci-robot merged commit 217844f into kubernetes-sigs:main Jun 15, 2022

Xenwar mentioned this pull request Jun 19, 2022

Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate #1257

Closed

bavarianbidi mentioned this pull request Jun 23, 2022

✨ Add v1alpha6 API types #1272

Merged

15 tasks

apricote mentioned this pull request Aug 24, 2022

✨ Allow for omitting AZ from control plane nodes #1318

Merged

3 tasks

bavarianbidi mentioned this pull request Sep 7, 2022

nil pointer if osc.status is empty (clusterctl move) and apiserver access is restricted #1339

Closed

nikParasyr mentioned this pull request Jan 12, 2023

Cluster deployment fails with 0.7.0-rc.1 and allowedCidrs set #1438

Closed

	routerIPs := []string{}
	for _, ip := range router.GatewayInfo.ExternalFixedIPs {
	routerIPs = append(routerIPs, ip.IPAddress)
	}

	openStackCluster.Status.Network.Router = &infrav1.Router{
	Name: router.Name,
	ID: router.ID,
	Tags: router.Tags,
	IPs: routerIPs,
	}

	if len(openStackCluster.Spec.APIServerLoadBalancer.AllowedCIDRs) > 0 {
	allowedCIDRs = append(allowedCIDRs, openStackCluster.Spec.APIServerLoadBalancer.AllowedCIDRs...)

	if openStackCluster.Spec.Bastion.Enabled {
	allowedCIDRs = append(allowedCIDRs, openStackCluster.Status.Bastion.FloatingIP, openStackCluster.Status.Bastion.IP)
	}

	if openStackCluster.Status.Network.Subnet.CIDR != "" {
	allowedCIDRs = append(allowedCIDRs, openStackCluster.Status.Network.Subnet.CIDR)
	}

	if len(openStackCluster.Status.Network.Router.IPs) > 0 {
	allowedCIDRs = append(allowedCIDRs, openStackCluster.Status.Network.Router.IPs...)
	}
	}

	openStackCluster.Status.Network.APIServerLoadBalancer = &infrav1.LoadBalancer{
	Name: lb.Name,
	ID: lb.ID,
	InternalIP: lb.VipAddress,
	IP: lbFloatingIP,
	AllowedCIDRs: allowedCIDRs,
	}

	// Manually restore data.
	restored := &infrav1.OpenStackCluster{}
	if ok, err := utilconversion.UnmarshalData(r, restored); err != nil \|\| !ok {
	return err
	}

	// Preserve Hub data on down-conversion except for metadata
	if err := utilconversion.MarshalData(src, r); err != nil {
	return err
	}

✨ Feature: restrict API Server LB access via IPs #1247

✨ Feature: restrict API Server LB access via IPs #1247

Uh oh!

Conversation

bavarianbidi commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Behavior:

Conclusion:

Open questions:

Uh oh!

netlify bot commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Uh oh!

k8s-ci-robot commented May 23, 2022

Uh oh!

jichenjc commented May 23, 2022

Uh oh!

jichenjc commented May 27, 2022

Uh oh!

bavarianbidi commented May 27, 2022

Uh oh!

jichenjc commented May 30, 2022

Uh oh!

bavarianbidi commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanschneeweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiasgiese left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jichenjc commented Jun 13, 2022

Uh oh!

tobiasgiese commented Jun 13, 2022

Uh oh!

k8s-ci-robot commented Jun 13, 2022

Uh oh!

bavarianbidi commented Jun 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bavarianbidi commented May 23, 2022 •

edited

Loading

netlify bot commented May 23, 2022 •

edited

Loading

bavarianbidi commented May 30, 2022 •

edited

Loading